Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Add compatibility to redis > 6 ACLs system using username in queue-mode #5048

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

intel44
Copy link
Contributor

@intel44 intel44 commented Dec 28, 2022

Github issue / Community forum post (link here to close automatically): https://community.n8n.io/t/add-a-configuration-variable-for-the-redis-username-queue-mode/15878

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2022

CLA assistant check
All committers have signed the CLA.

@n8n-assistant n8n-assistant bot added community Authored by a community member core Enhancement outside /nodes-base and /editor-ui labels Dec 28, 2022
packages/cli/src/Queue.ts Outdated Show resolved Hide resolved
@netroy
Copy link
Member

netroy commented Dec 28, 2022

@intel44 Have you tested this in the queue mode?
I assume that we need to update ioredis to 4.x or 5.x to be able to use any new redis features.

@intel44
Copy link
Contributor Author

intel44 commented Dec 28, 2022

Hi @netroy, thanks for your feedback.

Have you tested this in the queue mode?
I assume that we need to update ioredis to 4.x or 5.x to be able to use any new redis features.

After verification, no need to update any package. The current versions are fully compatible without adjustment.

Current packages :

The authentication feature has been include to [email protected] : redis/ioredis#1130


Some tests to ensure compatibility

Config

My redis users for tests :

$ redis-cli
127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
2) "user user-with-password on #a1159e9df3670d549d04524532629f5477ceb7deec9b45e47e8c009506ecb2c8 ~* &* +@all"
3) "user user-without-password on nopass ~* &* +@all"

Tests

I added a log of redisConfig in Queue.js to show the config used in queue mode.

Default mode

$ pnpm worker

> [email protected] worker /var/www/n8n
> ./packages/cli/bin/n8n worker

Starting n8n worker...
{ host: 'localhost', port: 6379, db: 80 }

n8n worker is now ready
 * Version: 0.209.3
 * Concurrency: 10

Specific user without password

$ export QUEUE_BULL_REDIS_USERNAME=user-without-password 
$ pnpm worker

> [email protected] worker /var/www/n8n
> ./packages/cli/bin/n8n worker

Starting n8n worker...
{
  host: 'localhost',
  username: 'user-without-password',
  port: 6379,
  db: 80
}

n8n worker is now ready
 * Version: 0.209.3
 * Concurrency: 10

Specific user with password

$ export QUEUE_BULL_REDIS_USERNAME=user-with-password 
$ export QUEUE_BULL_REDIS_PASSWORD=pwd
$ pnpm worker

> [email protected] worker /var/www/n8n
> ./packages/cli/bin/n8n worker

Starting n8n worker...
{
  host: 'localhost',
  username: 'user-with-password',
  password: 'pwd',
  port: 6379,
  db: 80
}

n8n worker is now ready
 * Version: 0.209.3
 * Concurrency: 10

Rotten case : Specific user with bad password

$ export QUEUE_BULL_REDIS_PASSWORD=pwdbad
$ pnpm worker

> [email protected] worker /var/www/n8n
> ./packages/cli/bin/n8n worker

Starting n8n worker...
{
  host: 'localhost',
  username: 'user-with-password',
  password: 'pwdbad',
  port: 6379,
  db: 80
}

n8n worker is now ready
 * Version: 0.209.3
 * Concurrency: 10

Error from queue: 

/var/www/n8n/node_modules/.pnpm/[email protected]/node_modules/redis-parser/lib/parser.js:179
    return new ReplyError(string)
           ^
ReplyError: WRONGPASS invalid username-password pair or user is disabled.
    at parseError (/var/www/n8n/node_modules/.pnpm/[email protected]/node_modules/redis-parser/lib/parser.js:179:12)
    at parseType (/var/www/n8n/node_modules/.pnpm/[email protected]/node_modules/redis-parser/lib/parser.js:302:14)
 ELIFECYCLE  Command failed with exit code 1.

Rotten case : Unknow user

$ export QUEUE_BULL_REDIS_USERNAME=user-unknow          
$ pnpm worker

> [email protected] worker /var/www/n8n
> ./packages/cli/bin/n8n worker

Starting n8n worker...
{ host: 'localhost', username: 'user-unknow', port: 6379, db: 80 }

n8n worker is now ready
 * Version: 0.209.3
 * Concurrency: 10

Error from queue: 

/var/www/n8n/node_modules/.pnpm/[email protected]/node_modules/redis-parser/lib/parser.js:179
    return new ReplyError(string)
           ^
ReplyError: WRONGPASS invalid username-password pair or user is disabled.
    at parseError (/var/www/n8n/node_modules/.pnpm/[email protected]/node_modules/redis-parser/lib/parser.js:179:12)
    at parseType (/var/www/n8n/node_modules/.pnpm/[email protected]/node_modules/redis-parser/lib/parser.js:302:14)
 ELIFECYCLE  Command failed with exit code 1.

@intel44 intel44 force-pushed the redis-add-username-acl-compatibility branch from 0495bf9 to 77e29ff Compare December 28, 2022 23:18
@intel44 intel44 changed the title feat(redis): add compatibility to redis > 6 ACLs system using username feat(cli): add compatibility to redis > 6 ACLs system using username in queue-mode Dec 29, 2022
@StarfallProjects
Copy link
Contributor

If/when this gets merged, can someone either give me a heads-up, or also merge the related docs PR? n8n-io/n8n-docs#1055 (and check it's still correct, if there are further code changes)

@netroy netroy changed the title feat(cli): add compatibility to redis > 6 ACLs system using username in queue-mode feat(core): Add compatibility to redis > 6 ACLs system using username in queue-mode Jan 3, 2023
@netroy netroy merged commit 0ec66bf into n8n-io:master Jan 3, 2023
@netroy netroy added the Upcoming Release Will be part of the upcoming release label Jan 3, 2023
@netroy
Copy link
Member

netroy commented Jan 3, 2023

@StarfallProjects both PRs merged

if (redisDB) {
redisOptions.db = redisDB;
}
const redisOptions: RedisOptions = config.getEnv('queue.bull.redis');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @netroy
With this commit, you remove my modifications: when the variable QUEUE_BULL_REDIS_USERNAME is not set, it will send username: ''
Is this going to be a problem ?

Copy link
Member

@netroy netroy Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't seem to be an issue when I tested it on redis 5 and 6.

@janober
Copy link
Member

janober commented Jan 5, 2023

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member core Enhancement outside /nodes-base and /editor-ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants